added ability for callbacks to have time.Time as arguments #604
Conversation
|
If the features is acceptable then if required I will add tests |
callback.go
Outdated
| var err error | ||
| c := (*C.char)(unsafe.Pointer(C.sqlite3_value_text(v))) | ||
| tv := C.GoString(c) | ||
| s := strings.TrimSuffix(tv, "Z") |
There was a problem hiding this comment.
The sqlite3_value is coming from the database. So can I assume it will not have timezone in it ?
There was a problem hiding this comment.
https://www.sqlite.org/lang_datefunc.html
SQLite does support time strings with time zone.
See section Time Strings
|
please add test. |
sum12
force-pushed
the
timecallback
branch
2 times, most recently
from
2fbbed2
to
ddb17e0
Compare
|
I have added tests. but I dont understand why the callbacks are failing for |
sum12
force-pushed
the
timecallback
branch
2 times, most recently
from
3d52bcb
to
cba271c
Compare
|
okay that should do it. Let me know what you think |
|
Any reviews on this one ? |
|
Seems good to me. cc @gjrtimmer |
|
thanks for fixing travis, |
|
@gjrtimmer do you have any more reviews? |
|
In recent changes, time.Time can be passed correctly. So IMO, this hack is not required. @sum12 Do you think? |
|
Could you please point me to the commit that added this functionality? I looked at callback.go and it doesnot have it. Or may be you could help me convert this hack to something that is not a hack ? |
|
@sum12 I do have a few comments on your changes.
|
go-sqlite3 can store time.Time values, and also has the fetaure to register callback function to provide additionaly functions. However these callback cannot take the mentioned time.Time columns as input as there is no "dynamic-casting" of these values from TEXT to time.Time. With this patch that become possible letting user write custom functions to do fancy filtering these time.Time columns
done
hrm, done.
this one was good for learning, thanks.
make sense, corrected.
the library does not recognize the other formats, only
Accepted and corrected Thanks for reviews, however I am still learning Go. Do you see anything else not in line ? |
|
@rittneje This is acceptable? |
| "unsafe" | ||
| ) | ||
|
|
||
| var timetype = reflect.TypeOf(time.Now()) |
There was a problem hiding this comment.
You should just do reflect.TypeOf(time.Time{}). No need to take the hit of actually fetching the system times at startup.
| @@ -243,10 +246,40 @@ func callbackArg(typ reflect.Type) (callbackArgConverter, error) { | |||
| c := callbackArgCast{callbackArgFloat64, typ} | |||
| return c.Run, nil | |||
| default: | |||
| return nil, fmt.Errorf("don't know how to convert to %s", typ) | |||
| switch typ { | |||
| case timetype: | |||
There was a problem hiding this comment.
It is possible that the callback arg is not time.Time directly, but rather a typedef on it (e.g., type Foo time.Time). If we want to support such things, then instead you want to check if time.Time is convertible to typ (using the ConvertibleTo method).
On that note, it looks like there are actually bugs in the existing logic on this point. For instance, if I have a callback that takes a string typedef (e.g., type Bar string), that will be allowed, because the kind of that type is still reflect.String. But later the call will lead to a runtime panic. https://play.golang.org/p/MI-324y6veS
Fixing that will require appropriate calls to Convert. For example: https://play.golang.org/p/k77Y82MDNeL
The problem is even more interesting for []byte (aka []uint8), because you cannot actually convert between []byte and []TypedefOnByte. That particular case should probably be fixed to just check if the arg kind is not string and []byte is convertible to the arg type (or the arg type is specifically []byte) rather than doing a kind-based check.
To be clear, I am not suggesting you fix the existing bugs in this PR (although that would be very much appreciated). Just that we should decide whether such typedefs ought to be rejected - in which case your new code is fine as-is - or accepted - in which case we need the extra calls to ConvertibleTo and Convert. @mattn What are your thoughts on this point?
go-sqlite3 can store time.Time values, and also has the fetaure to
register callback function to provide additionaly functions.
However these callback cannot take the mentioned time.Time columns as
input as there is no "dynamic-casting" of these values from TEXT to
time.Time.
With this patch that become possible letting user write custom functions
to do fancy filtering these time.Time columns